Include err.message for debug data - #11
Conversation
|
@bajtos PTAL |
| // NOTE err.stack is not an enumerable property | ||
| // NOTE `err.stack` and `err.message` is not an enumerable property | ||
| data.stack = err.stack; | ||
| if (err.message) |
There was a problem hiding this comment.
We don't check whether err.message is set in other fill functions, is there any reason why we can't do the same here?
I'd also prefer to set the message among the first items, because JSON serializers preserve the order in which properties were defined.
data.message = err.message;
for (var p in err) {
if ((p in data)) continue;
data[p] = err[p];
}
data.stack = err.stack;Could you please check that err.name is included in debug-mode data too?
There was a problem hiding this comment.
if we do data.message = err.message;
the result will come back with
"message" : ""
and we need to handle scenario where a non-error is thrown (eg. throw 'string' )
https://github.com/strongloop/strong-error-handler/blob/ff2af3b5555ee84bdb110fef74effbf891a176fa/lib/data-builder.js#L20
do you think data.message = data.message || err.message; would work?
There was a problem hiding this comment.
the result will come back with
"message" : ""
Hmm, I did was not aware of this edge case. In that case, I guess the following would be best?
if (err.message && !data.message)
data.message = err.message;There was a problem hiding this comment.
I think we need to handle this edge case in fillBadRequestError too, shouldn't we?
Another option, which may be better, is to change the code handling non-errors so that it modified the err object instead of the data object:
if (typeof err !== 'object') {
err = {
statusCode: 500,
message: '' + err,
}
}And then we can do data.message = err.message everywhere else.
@davidcheung Thoughts?
deda5be to
be5dc33
Compare
|
@davidcheung could you please add a testcase (or two?) to cover "the result will come back with Also cross-posting from collapsed #11 (diff)
Hmm, I did was not aware of this edge case. In that case, perhaps the following would be better? if (err.message && !data.message)
data.message = err.message;I think we need to handle this edge case in fillBadRequestError too, shouldn't we? Another option, which may be even better, is to change the code handling non-errors so that it modified the if (typeof err !== 'object') {
err = {
statusCode: 500,
message: '' + err,
}
}And then we can do @davidcheung Thoughts? |
292d5ac to
df1f9dc
Compare
|
agreed, i think this approach is better too
if (typeof err !== 'object') {
err = {
statusCode: 500,
message: '' + err,
}
}this test case should handle the
|
e700850 to
53dc723
Compare
|
another issue to bring up is we are nolonger removing the |
a7c4d38 to
1c94fa7
Compare
f845a74 to
42613b7
Compare
Probably not intentional, please open a new pull request. |
| data.message = '' + err; | ||
| err = {}; | ||
| err = { | ||
| name: 'Error', |
There was a problem hiding this comment.
Can we omit name in this case? Will it cause any failures in existing tests, either here or in strong-remoting?
|
@bajtos I have addressed the comments, PTAL |
| requestJson().end(function(err, res) { | ||
| if (err) return done(err); | ||
| expect(res.body).to.have.property('error'); | ||
| expect(res.body.error.name).to.eql('Error'); |
There was a problem hiding this comment.
expect(res.body).to.have.property('error', 'Error');
|
Two more nitpicks, the rest LGTM. No further reviews are necessary, just wait for CI pass and squash the commits before landing. |
err.message and err.name are not enumerable, therefore needs to be explicited added to the data obj
384befc to
ef72b5c
Compare
2 participants
Because err.message is also not enumerable,
therefore needs to be explicited added to the data obj